Make PTBKey not extend ReferenceBinding #3452

jukzi · 2024-12-13T11:40:30Z

Introduces interface TypeBindingWrapper.
Removes little code from PTBKey that is irrelevant for the wrapper

relates to #3412

Its just an IdentityWrapper - do not rely on ReferenceBinding implementation. relates to eclipse-jdt#3452

jukzi · 2024-12-17T07:56:11Z

@srikanth-sankaran may i ask for review? I would like to improve ReferenceBinding in small steps.

srikanth-sankaran · 2024-12-17T09:48:45Z

@srikanth-sankaran may i ask for review? I would like to improve ReferenceBinding in small steps.

I'll try - but can't promise, I am trying to wrap up some projects I am working on before this week since post that I am away till 2nd Jan.

Its just an IdentityWrapper - do not rely on ReferenceBinding implementation. relates to #3452

jukzi · 2025-01-07T07:47:52Z

@srikanth-sankaran ping, please review.

srikanth-sankaran · 2025-01-07T08:52:38Z

Sure, I will look into these this week. Happy new year!

srikanth-sankaran · 2025-01-09T09:50:40Z

Sure, I will look into these this week. Happy new year!

Sorry, it looks like my full return to work will be delayed until a few days more. I will be at work 3 out of 5 days next week and I am hopeful the review can be completed well ahead of M2.

srikanth-sankaran

See comments, need some clarifications on what exactly we are trying to accomplish here.

I am yet to come to grips with what the new additional interface buys us.

Also the title of the PR PTBKey is not a ReferenceBinding - need clarification. What exactly do you mean by that.

I don't see anything seriously wrong/faulty - but simply don't get yet where we are going with this and why

...jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeBindingWrapper.java

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeBinding.java

...eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java

jukzi · 2025-01-17T09:49:10Z

....compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/UnresolvedReferenceBinding.java

@@ -53,18 +53,18 @@ public TypeBinding clone(TypeBinding outerType) {
 	return copy;
 }

-void addWrapper(TypeBinding wrapper, LookupEnvironment environment) {
+void addWrapper(TypeBindingWrapper wrapper, LookupEnvironment environment) {


This is the only usage of the proposed interface. Here the instance is called "wrapper" and the method name add"Wrapper" also suggest to have "Wapper" in the interface name. Thats why i chose "TypeBindingWrapper". but i would be fine with any other Name.

srikanth-sankaran · 2025-01-17T10:04:16Z

Not sure if you are planning to explain the PR title change - the new one still doesn't clear the mystery for me :)

Do you have a scenario where the PTBKey is actually not a reference binding ??

Without the additional name TypeBindingWrapper being used, things were working so far - so what does extracting the swapUnresolved behavior into its own type buy us ?

jukzi · 2025-01-17T10:10:16Z

Without the additional name TypeBindingWrapper being used, things were working so far - so what does extracting the swapUnresolved behavior into its own type buy us ?

I am trying to clean up the ReferenceBinding equals/hashCode contract as it hard to understand and errorprone to accidentialy use when using a ReferenceBinding as key (for example #3561 ). As this PTBKey does not need to implement ReferenceBinding one could just ignore the PTBKey in such scenarios. At the end it will be fine if the PTBKey will have a custom equals/hashcode, but any actual ReferenceBinding probably won't need to override equals/hashcode => no more worries in the future.

srikanth-sankaran · 2025-01-17T10:15:18Z

Without the additional name TypeBindingWrapper being used, things were working so far - so what does extracting the swapUnresolved behavior into its own type buy us ?

I am trying to clean up the ReferenceBinding equals/hashCode contract as it hard to understand and errorprone to accidentialy use when using a ReferenceBinding as key (for example #3561 ). As this PTBKey does not need to implement ReferenceBinding one could just ignore the PTBKey in such scenarios. At the end it will be fine if the PTBKey will have a custom equals/hashcode, but any actual ReferenceBinding probably won't need to override equals/hashcode => no more worries in the future.

PTBKey is not "implementing" ReferenceBinding but ISA ReferenceBinding - do you know of cases where it is NOT a ReferenceBinding ? If not this whole PR will become moot and will only serve to obfuscate by introducing additional names...

jukzi · 2025-01-17T10:26:26Z

PTBKey is not "implementing" ReferenceBinding but ISA ReferenceBinding - do you know of cases where it is NOT a ReferenceBinding ? If not this whole PR will become moot and will only serve to obfuscate by introducing additional names...

It was a ReferenceBinding but did not need to be ReferenceBinding. It is only a wrapper used as key for ParameterizedTypeBinding. It is not used as ReferenceBinding itself anywhere. Especially PTBKey.hash(TypeBinding) will never call PTBKey.hashCode() which was not clear from the type hierarchy.

srikanth-sankaran · 2025-01-20T04:38:52Z

PTBKey is not "implementing" ReferenceBinding but ISA ReferenceBinding - do you know of cases where it is NOT a ReferenceBinding ? If not this whole PR will become moot and will only serve to obfuscate by introducing additional names...

It was a ReferenceBinding but did not need to be ReferenceBinding. It is only a wrapper used as key for ParameterizedTypeBinding. It is not used as ReferenceBinding itself anywhere. Especially PTBKey.hash(TypeBinding) will never call PTBKey.hashCode() which was not clear from the type hierarchy.

OK, I finally have an understanding of what you are trying to do. With that in mind, taking a fresh relook ...

srikanth-sankaran · 2025-01-20T05:07:48Z

I have reviewed the fix and I don't have major objections to it.

Yes, the PTBKey behavior does not call for it to be a ReferenceBinding per se and to factor out the relevant behavior into an interface is OK.

(1) I am still not very happy with the name - Resolvable is way better than TypeBindingWrapper which was plain WRONG. In the original patch, TypeBinding was implementing TypeBindingWrapper - That is wrong. TypeBinding is actually a Wrappable and not a Wrapper (but see below)

But Resolvable is not precise enough. I wonder if a name like HotSwappable is more to the point. Sorry for overriding my own earlier suggestion and causing rework.

(2) A TypeBinding itself is not HotSwappable - ONLY those abstractions in the hierarchy that implement the method org.eclipse.jdt.internal.compiler.lookup.TypeBinding.swapUnresolved(UnresolvedReferenceBinding, ReferenceBinding, LookupEnvironment) are. That presently TypeBinding has a dummy/nop/placeholder implementation does not count.

I think this PR may look a lot better

- if `Resolvable` is renamed to be `HotSwappable` and
- if  `HotSwappable` is implemented ONLY by `ArrayBinding`, `ParameterizedTypeBinding`, `WildCardBinding`, `PTBKey` and `UnresolvedReferenceBinding` 
- if `TypeBinding` itself DOES NOT implement it

What do you think ? If you agree, you may proceed to make the changes and integrate once tests are green. Thanks

PTBKey needs a custom hashCode/equals contract, while ReferenceBinding should not need any. * PTBKey not extends ReferenceBinding anymore. * Introduces interface HotSwappable. * Removes little code from PTBKey that is irrelevant for the wrapper relates to eclipse-jdt#3412

jukzi · 2025-01-20T08:38:15Z

What do you think ? If you agree, you may proceed to make the changes and integrate once tests are green. Thanks

sounds good, changes applied

srikanth-sankaran

Changes look good. Please proceed to integrate assuming no surprises in testing.

jukzi · 2025-01-20T09:34:31Z

the failing test is known to randomly fail: #2716 (comment)

jukzi requested a review from srikanth-sankaran December 13, 2024 11:45

jukzi pushed a commit to jukzi/eclipse.jdt.core that referenced this pull request Dec 13, 2024

ReferenceBindingSetWrapper: use System.identityHashCode

32211a7

Its just an IdentityWrapper - do not rely on ReferenceBinding implementation. relates to eclipse-jdt#3452

jukzi mentioned this pull request Dec 13, 2024

ReferenceBindingSetWrapper: use System.identityHashCode #3453

Merged

jukzi pushed a commit that referenced this pull request Dec 17, 2024

ReferenceBindingSetWrapper: use System.identityHashCode

5e40502

Its just an IdentityWrapper - do not rely on ReferenceBinding implementation. relates to #3452

srikanth-sankaran reviewed Jan 17, 2025

View reviewed changes

jukzi commented Jan 17, 2025

View reviewed changes

jukzi changed the title ~~PTBKey is not a ReferenceBinding~~ PTBKey does not need to implement ReferenceBinding Jan 17, 2025

jukzi force-pushed the TypeBindingWrapper branch from 54187ac to 068f092 Compare January 17, 2025 10:00

jukzi force-pushed the TypeBindingWrapper branch from 068f092 to cf810f5 Compare January 17, 2025 10:13

jukzi changed the title ~~PTBKey does not need to implement ReferenceBinding~~ Make PTBKey not extend ReferenceBinding Jan 17, 2025

jukzi force-pushed the TypeBindingWrapper branch from cf810f5 to c633101 Compare January 20, 2025 08:37

srikanth-sankaran approved these changes Jan 20, 2025

View reviewed changes

jukzi merged commit 121e230 into eclipse-jdt:master Jan 20, 2025
7 of 10 checks passed

jukzi deleted the TypeBindingWrapper branch January 20, 2025 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PTBKey not extend ReferenceBinding #3452

Make PTBKey not extend ReferenceBinding #3452

jukzi commented Dec 13, 2024

jukzi commented Dec 17, 2024 •

edited

Loading

srikanth-sankaran commented Dec 17, 2024

jukzi commented Jan 7, 2025

srikanth-sankaran commented Jan 7, 2025

srikanth-sankaran commented Jan 9, 2025

srikanth-sankaran left a comment •

edited

Loading

jukzi Jan 17, 2025

srikanth-sankaran commented Jan 17, 2025

jukzi commented Jan 17, 2025

srikanth-sankaran commented Jan 17, 2025

jukzi commented Jan 17, 2025

srikanth-sankaran commented Jan 20, 2025

srikanth-sankaran commented Jan 20, 2025 •

edited

Loading

jukzi commented Jan 20, 2025

srikanth-sankaran left a comment

jukzi commented Jan 20, 2025

Make PTBKey not extend ReferenceBinding #3452

Make PTBKey not extend ReferenceBinding #3452

Conversation

jukzi commented Dec 13, 2024

jukzi commented Dec 17, 2024 • edited Loading

srikanth-sankaran commented Dec 17, 2024

jukzi commented Jan 7, 2025

srikanth-sankaran commented Jan 7, 2025

srikanth-sankaran commented Jan 9, 2025

srikanth-sankaran left a comment • edited Loading

Choose a reason for hiding this comment

jukzi Jan 17, 2025

Choose a reason for hiding this comment

srikanth-sankaran commented Jan 17, 2025

jukzi commented Jan 17, 2025

srikanth-sankaran commented Jan 17, 2025

jukzi commented Jan 17, 2025

srikanth-sankaran commented Jan 20, 2025

srikanth-sankaran commented Jan 20, 2025 • edited Loading

jukzi commented Jan 20, 2025

srikanth-sankaran left a comment

Choose a reason for hiding this comment

jukzi commented Jan 20, 2025

jukzi commented Dec 17, 2024 •

edited

Loading

srikanth-sankaran left a comment •

edited

Loading

srikanth-sankaran commented Jan 20, 2025 •

edited

Loading